Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better help organization for --querytags and --queryinfo #1627

Merged
merged 1 commit into from
Jun 25, 2020

Conversation

abitrolly
Copy link
Contributor

This makes help output self-sufficient, and removes extra doc form --querytags output making it possible to parse the output with scripts (rpm-software-management/dnf5#1243)

New help output.

...
  --qf QUERYFORMAT, --queryformat QUERYFORMAT
                        format for showing found packages: "%{name} %{version} ..".
                        Use --querytags to view full tag list
  --querytags           show available tags to use with --queryformat
...

Copy link
Contributor

@lukash lukash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch. I haven't actually realized until now the argparser doesn't sort the options alphabetically 😎

dnf/cli/commands/repoquery.py Outdated Show resolved Hide resolved
dnf/cli/commands/repoquery.py Outdated Show resolved Hide resolved
@@ -435,7 +435,6 @@ def _add_add_remote_packages(self):

def run(self):
if self.opts.querytags:
print(_('Available query-tags: use --queryformat ".. %{tag} .."'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine personally with removing this, but I'd like a second opinion just to make sure someone doesn't find it actually userful, @dmach @j-mracek @m-blaha, do you agree with removing this line?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also if you wanna do this, could you also remove at least the leading line-break (and ideally the trailing one too) from the QUERY_TAGS variable? Now it prints an empty line before and after the list of tags which are superfluous.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed spaces. Although I would also split reponame (repoid) into separate items, or removed reponame completely. Then it will be relatively easy to parse the output with something like bash.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears reponame doesn't stand for the name, it's the id, same as repoid. An obvious error? I'm not against just removing it, but I'm not sure we can do that. I'm sure @j-mracek will know more though?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry, but I do not understanding your question. Reponame as an attribute of package class. --qf is capable to return any attribute of Package class.

  .. attribute:: reponame

    Id of repository the package was installed from (string).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry, but I do not understanding your question. Reponame as an attribute of package class. --qf is capable to return any attribute of Package class.

The question is whether we should remove reponame from the list, because it contains the ID, not the name. And since there already is repoid, it is redundant in addition to wrong.

  .. attribute:: reponame

    Id of repository the package was installed from (string).

I'd assume this is some sort of legacy API attribute, since it's called "name" and contains the ID. FTR the excerpt is from the API documentation: https://github.com/rpm-software-management/dnf/blob/master/doc/api_package.rst. Interestingly repoid isn't listed there and also no way to get the actual repo name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any benefit in removal of print(_('Available query-tags: use --queryformat ".. %{tag} .."'))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@j-mracek it is to make the output suitable for scripting. For example https://github.com/rpm-software-management/libdnf/issues/720#issuecomment-631625069

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I'm not entirely sure what would you parse the list for (ultimately you have to know the semantics of each field so you need to have them listed somewhere in your code anyway), I find it reasonable to remove it...

@lukash lukash self-assigned this May 25, 2020
@abitrolly
Copy link
Contributor Author

Sorting options automatically is hard when subgroups are involved as in this case. )

Bring commands next to each other and make --querytags
output machine readable.
@lukash
Copy link
Contributor

lukash commented Jun 25, 2020

@j-mracek this is stuck on your input so I'll reassign it to you. If you'd be fine with the changes and just want me to finish this up, feel free to assign back to me. Thanks.

@lukash lukash assigned j-mracek and unassigned lukash Jun 25, 2020
@abitrolly abitrolly requested a review from j-mracek June 25, 2020 11:22
@j-mracek
Copy link
Contributor

@rh-atomic-bot r+

@rh-atomic-bot
Copy link

📌 Commit c1aedfe has been approved by j-mracek

@rh-atomic-bot
Copy link

⌛ Testing commit c1aedfe with merge b0b015c...

rh-atomic-bot pushed a commit that referenced this pull request Jun 25, 2020
Bring commands next to each other and make --querytags
output machine readable.

Closes: #1627
Approved by: j-mracek
@rh-atomic-bot
Copy link

☀️ Test successful - status-papr
Approved by: j-mracek
Pushing b0b015c to master...

@rh-atomic-bot
Copy link

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

@j-mracek j-mracek merged commit 37f715c into rpm-software-management:master Jun 25, 2020
@abitrolly abitrolly deleted the qf-cli-help branch June 25, 2020 14:42
@abitrolly
Copy link
Contributor Author

@lukash @j-mracek thanks.

I've opened https://bugzilla.redhat.com/show_bug.cgi?id=1851125 for dealing with the reponame and repoid extra.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants